Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[C64] Fix memory performance issues #4152

Merged
merged 3 commits into from
Jan 7, 2025
Merged

[C64] Fix memory performance issues #4152

merged 3 commits into from
Jan 7, 2025

Conversation

SaxxonPike
Copy link
Contributor

@SaxxonPike SaxxonPike commented Dec 31, 2024 •
edited
Loading

Addresses #4151

Causes:

  • "Save RAM modified" flag is always True
  • Deltas are calculated each frame due to the aforementioned flag
  • Excess array allocations in the C64 core during delta calculation

Solutions:

  • Fewer runtime array allocations (re-use buffers)
  • Proper implementation of "save RAM modified" flag (only set when the disk data has changed since the last save)
  • Instead of tracking which disk tracks are used at all, track which disk tracks are modified
  • Store deltas only for tracks that have been modified

Practical tests run:

  • Performance monitoring
  • Load and save state using C64 disk media (D64, PRG)
  • Load and save state on some other cores

Before changes, using Remember's release of "Uridium":
before-changes
After changes, using the same disk:
after-changes

Check if completed:

CasualPokePlayer
CasualPokePlayer previously requested changes Dec 31, 2024
View reviewed changes
src/BizHawk.Common/DeltaSerializer.cs Outdated Show resolved Hide resolved
YoshiRulz
YoshiRulz requested changes Dec 31, 2024
View reviewed changes
Copy link
Member

@YoshiRulz YoshiRulz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What part of this is using ArrayPool? If you scrapped all of this and simply swapped out allocations for ArrayPool, how effective would that be?

src/BizHawk.Common/Util.cs Outdated Show resolved Hide resolved
src/BizHawk.Common/Util.cs Outdated Show resolved Hide resolved
src/BizHawk.Common/Util.cs Outdated Show resolved Hide resolved
src/BizHawk.Emulation.Cores/Computers/Commodore64/Cartridge/Mapper0020.cs Outdated Show resolved Hide resolved
src/BizHawk.Emulation.Cores/Computers/Commodore64/Serial/Drive1541.cs Outdated Show resolved Hide resolved
src/BizHawk.Common/Serializer.cs Outdated Show resolved Hide resolved
src/BizHawk.Common/Serializer.cs Outdated Show resolved Hide resolved
src/BizHawk.Emulation.Cores/Computers/Commodore64/Media/Disk.cs Outdated Show resolved Hide resolved
@SaxxonPike
Copy link
Contributor Author

Thanks for the feedback. Having stepped away and come back to it, I think the scope of these changes is larger than it needs to be. I'm going to put in refinements this week.

YoshiRulz
YoshiRulz approved these changes Jan 4, 2025
View reviewed changes
Copy link
Member

@YoshiRulz YoshiRulz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rebase again and I think this is good. And please update the OP, including your estimate for the performance boost.

@SaxxonPike
Copy link
Contributor Author

Performance results added. The results are similar across all tested disks- I just used one as an example.

@YoshiRulz
Copy link
Member

YoshiRulz commented Jan 4, 2025 •
edited
Loading

Do you need help with rebasing? I was expecting

p 4f34320b3
p abf2014d3
p d6223e8ef
f ff92bddb5
p d9d629e1b

Nice charts BTW.

@YoshiRulz
Copy link
Member

YoshiRulz commented Jan 6, 2025 •
edited
Loading

Did you intend to do something like this? (On that branch I've also made a few code style fixes, to save having to leave more review comments.)

@SaxxonPike
Copy link
Contributor Author

Yeah, I think some of the branch history got a bit muddled. That looks fine to me. For posterity, the commit list should look more like this: https://github.com/SaxxonPike/BizHawk/tree/disk-delta-fixes-merge

@YoshiRulz
Copy link
Member

Would you like to force-push that here then?

@YoshiRulz YoshiRulz merged commit 0adf2f9 into TASEmulators:master Jan 7, 2025
4 checks passed
@SaxxonPike
Copy link
Contributor Author

@YoshiRulz Thank you for taking care of this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@YoshiRulz YoshiRulz YoshiRulz approved these changes

@CasualPokePlayer CasualPokePlayer CasualPokePlayer left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@SaxxonPike @YoshiRulz @CasualPokePlayer